Skip to content

test(desktop): deflake Command Code browser login port release - #5479

Open
orangeCatDeveloper wants to merge 1 commit into
apache:mainfrom
orangeCatDeveloper:test/commandcode-browser-login-port-flake
Open

test(desktop): deflake Command Code browser login port release#5479
orangeCatDeveloper wants to merge 1 commit into
apache:mainfrom
orangeCatDeveloper:test/commandcode-browser-login-port-flake

Conversation

@orangeCatDeveloper

@orangeCatDeveloper orangeCatDeveloper commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

CommandCodeBrowserLoginController tests intermittently flaked on an occupied port is skipped for the next one in the CLI range with 46961 !== 46960. Two defects caused the port collision:

  1. isPortFree called occupyPort, unintentionally retaining an active net.Server in netServers until afterEach.
  2. Tests completing or settling attempts (notably two simultaneous starts) did not await the winner's loopback port release before ending, racing the subsequent test against OS socket teardown.

Refs #5395

Verification

Numbers (before -> after):

  • Leaked probe servers in netServers: 1 -> 0 (isPortFree immediately closes upon verifying instead of holding the port until afterEach).
  • Winner port release verification in two simultaneous starts: unawaited (racing) -> 100% awaited (assert.equal(await isPortFree(Number(callback.port)), true)).
  • Suite stability: flaked in CI run 35314799012 (46961 !== 46960) -> 50/50 passed (0 failures).
# Before (CI run 35314799012):
✖ an occupied port is skipped for the next one in the CLI range (5.455341ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  46961 !== 46960

# After (50 consecutive runs):
✔ CommandCodeBrowserLoginController (324.882208ms)
ℹ tests 20
ℹ suites 2
ℹ pass 20
ℹ fail 0
50/50 passed

Checks run:

  • npm run format:check (biome passed)
  • npm run lint (biome passed)
  • npm run typecheck in apps/desktop (passed)
  • npm run check:tui-copy (ok)
  • node scripts/check-renderer-architecture.mjs --base origin/main in apps/desktop (passed)
  • npm run build:main and node --test "dist/main/__tests__/commandcode-browser-login.test.js" in apps/desktop (20/20 passed)

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: GitHub Copilot diagnosed the port race, updated probe behavior and teardown assertions, and verified test suites. Commit includes the Generated-by: GitHub Copilot trailer.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@orangeCatDeveloper
test(desktop): deflake Command Code browser login port release
540b368 
isPortFree previously called occupyPort, retaining an active net.Server in
netServers until afterEach rather than probing and releasing immediately.
Additionally, tests completing or settling attempts (notably the simultaneous
starts test) did not await the winner's loopback port release before entering
the next test, leading to EADDRINUSE races where the next test skipped the
closing port and unexpectedly bound a higher port number.

Generated-by: GitHub Copilot
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added the effort/S Under 100 readable lines label Sep 18, 2026
@orangeCatDeveloper

orangeCatDeveloper commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

@Astro-Han

hqhq1025
hqhq1025 reviewed Sep 25, 2026
View reviewed changes

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the current head. The only change is to apps/desktop/src/main/__tests__/commandcode-browser-login.test.ts: the port-free helper binds then closes a temporary server, and several completion paths now assert that the callback port can be rebound (:91-105,184-371). I found no substantiated P0–P3 defect in that test change.

This PR should not be merged as-is. Current main removed the Command Code GO provider and this entire test file in bd39677ec (#5545); git merge-tree reports a modify/delete conflict. The added assertions therefore exercise a feature no longer present on main. The current-head test and label checks passed and git diff --check was clean, but those do not resolve the obsolete test or merge conflict. I did not run local tests (Node 18/no installed dependencies); there is no application-code or schema/migration change. The author/maintainers should decide whether to close this PR rather than reintroduce the retired test.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hqhq1025 hqhq1025 hqhq1025 left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

effort/S Under 100 readable lines

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@orangeCatDeveloper @hqhq1025